Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

🤖 Copilot Code Review — PR #127877

Holistic Assessment

Motivation: This PR implements GetTypeLayout and GetArrayLayout in the managed cDAC DacDbiImpl, adds IsObjRef and GetInternalCorElementType to the RuntimeTypeSystem contract, and makes changes to native debugger event handling (marshalling, connection events, log switch messages). It also removes several contracts and features that have been superseded or are being reverted to legacy fallback (ObjectiveCMarshal, IsTrackedReferenceWithFinalizer, WaitSleepJoin thread state, VirtCallStubHeap traversal). The changes advance the cDAC implementation and clean up code that is no longer needed.

Approach: The approach is sound. The cDAC implementations closely mirror the native DAC logic, with thorough DEBUG cross-validation assertions that will catch discrepancies at runtime. The removals are clean — each removed feature is reverted to legacy fallback, ensuring no functionality loss.

Summary: ⚠️ Needs Human Review. The core implementations (GetTypeLayout, GetArrayLayout, IsObjRef, GetInternalCorElementType) appear correct for mainstream type categories and are well-tested with DEBUG cross-validation. However, there is a potential correctness gap for TypeDesc-based array elements (flagged by multiple reviewers), and the PR's scope mixes several concerns. A human reviewer familiar with the cDAC and debugger areas should verify the TypeDesc edge case and the GC nullable→non-nullable change.

Detailed Findings

✅ GetTypeLayout Implementation — Correct

The managed cDAC implementation in DacDbiImpl.cs correctly mirrors the native DAC logic in dacdbiimpl.cpp:

• boxOffset: rts.IsObjRef(typeHandle) ? 0u : (uint)_target.PointerSize correctly matches CorTypeInfo::IsObjRef_NoThrow(componentType) ? 0 : sizeof(TADDR). Verified that IsObjRef (checking signature element type for Class/Array/SzArray) produces the same result as native's IsObjRef_NoThrow on the internal element type — CorTypeInfo table confirms Class, Array, SzArray, String, and Object all have TYPE_GC_REF (sigparser.h:881-886, siginfo.cpp:104-124).
• numFields: The subtraction approach (GetNumInstanceFields(type) - GetNumInstanceFields(parent)) correctly gives introduced-only instance field count, matching native's ApproxFieldDescIterator(mt, INSTANCE_FIELDS).Count().
• type: IsString ? String : GetInternalCorElementType matches native exactly.
• DEBUG cross-validation: Thorough field-by-field assertions against legacy (lines 1717-1733).

✅ GetArrayLayout String Case — Correct

• firstElementOffset = pointerSize + 4 matches native's sizeof(TADDR) + 4 (MT pointer + string length field) at dacdbiimpl.cpp:7412.
• elementSize = sizeof(char) (2) matches native's hardcoded 2 at dacdbiimpl.cpp:7415.
• All other fields (countOffset, rankSize, numRanks, rankOffset) match native exactly.

⚠️ GetArrayLayout Array Case — TypeDesc Element Type Handling (flagged by 2 reviewers)

File: DacDbiImpl.cs:1770-1774

The native code (dacdbiimpl.cpp:7429-7430) calls hnd.GetMethodTable() on the element TypeHandle, which normalizes TypeDescs (pointer/fnptr types) to their underlying MethodTable. The cDAC uses rts.GetTypeParam() directly, which would return the TypeDesc handle as-is. This could produce different componentID.token1 values and potentially different componentType values for arrays of pointer or function pointer types.

Mitigation: This is caught by the DEBUG cross-validation (lines 1790-1808) and is an extremely rare edge case in practice (arrays of pointer/fnptr types). However, it is a correctness gap between the native and managed implementations. A human reviewer should determine whether this edge case needs to be handled now or can be deferred.

✅ IsObjRef and GetInternalCorElementType — Correct

• IsObjRef checks signature element type for Class | Array | SzArray, aligning with CorTypeInfo where those types have TYPE_GC_REF.
• GetInternalCorElementType correctly returns the underlying primitive type for enums by checking GetClassData().InternalCorElementType when signature type is ValueType, falling through for all other cases. Logic verified against the design doc pseudocode in RuntimeTypeSystem.md.

✅ Native MarshalManagedEvent / DeleteIPCEventHelper — Correct

• MarshalManagedEvent (process.cpp:9289-9320) only marshals szContent for DB_IPCE_FIRST_LOG_MESSAGE. Confirmed by Haiku agent analysis that szCategory is an EmbeddedIPCString (inline in the event struct) and does not require cross-process marshalling.
• DeleteIPCEventHelper (process.cpp:10628-10653) correctly cleans up only szContent — the only field that was marshalled. Handles partial marshalling gracefully.
• Connection event handlers (DB_IPCE_CREATE_CONNECTION, DB_IPCE_DESTROY_CONNECTION, DB_IPCE_CHANGE_CONNECTION) and DB_IPCE_LOGSWITCH_SET_MESSAGE use GetString() on inline string buffers, which is correct.

⚠️ GC Heap Nullable → Non-Nullable — Verify target configurations

Files: GCHeapWKS.cs, GCHeapSVR.cs (Data), IGCHeap.cs

MarkArray, NextSweepObj, BackgroundMinSavedAddr, BackgroundMaxSavedAddr changed from TargetPointer? to TargetPointer. The old WKS code used TryReadGlobalPointer (graceful on missing globals); the new code uses ReadGlobalPointer which throws if not available. The old SVR code used type.Fields.ContainsKey() guards; the new code reads unconditionally.

These fields exist only in background GC builds. While all mainstream .NET configurations have background GC, this could break for non-background-GC configurations. A human reviewer should confirm this is acceptable for all supported cDAC target configurations.

⚠️ Scope — Large mixed-concern PR

This PR bundles: (1) cDAC GetTypeLayout/GetArrayLayout + new RuntimeTypeSystem APIs, (2) removal of ObjectiveCMarshal/IsTrackedReferenceWithFinalizer/VCSHeapType, (3) native debugger changes (MarshalManagedEvent, connection events, log switch, DataBreakpoint context size), (4) GetPartialUserState revert + WaitSleepJoin removal, (5) GC heap nullable refactor, (6) Helix payload changes. While each piece appears correct individually, the breadth increases review difficulty. This is advisory, not blocking.

✅ Test Coverage — Good

New unit tests (IsObjRef_ReturnsExpectedValues in MethodTableTests.cs) and dump tests (GetTypeLayout_Object_CrossValidatesContract, GetArrayLayout_ObjectArray_CrossValidatesContract, GetArrayLayout_String_HasExpectedLayout, RuntimeTypeSystem_IsObjRef_AreConsistent) provide solid coverage for the mainline cases. The dump tests cross-validate against real runtime data. Tests for removed features (ObjectiveCMarshal, VirtCallStubHeap, GetPartialUserState) are appropriately deleted.

💡 Minor: Typo in comment

File: GCHeapSVR.cs — "Fields only exist segment GC builds" → "Fields only exist in segment GC builds".

Generated by Code Review for issue #127877 · ◷